fixed #11202/#11199/#11201/#12330/#12774 / refs #13508/#11200/#13506 - fixed various floating-point comparison regressions #7148
Conversation
|
This hacks in the previous behavior - see b0b3f7e#r136702783. The goal here is to at least collect all the test cases and add them in as TODO ones so this can be looked into properly. |
firewave
force-pushed
the
fp-comp-x
branch
4 times, most recently
from
68e448f to
1329d34
Compare
|
This should be complete now in covering the regressions of the commit in question. @pfultz2 please have a look The change does not fix all the regressions but greatly improved things. And unfortunately it re-introduces a false positive fixed in https://trac.cppcheck.net/ticket/11203. I have not looked further into this yet. |
| @@ -550,14 +551,14 @@ namespace ValueFlow | |||
| setTokenValue(parent, std::move(result), settings); | |||
| } else if (Token::Match(parent, "%op%")) { | |||
| if (Token::Match(parent, "%comp%")) { | |||
| if (!result.isFloatValue() && !value1.isIntValue() && !value2.isIntValue()) | |||
| if (!isFloat && !value1.isIntValue() && !value2.isIntValue()) | |||
There was a problem hiding this comment.
The !isFloat is redundant here.
There was a problem hiding this comment.
I thought the same but it is not (this is actually the important change based on my older comment). It reflects if either is a float. That doesn't mean one of them cannot be an Int.
There was a problem hiding this comment.
I see, To make it clearer dont use the isFloat variable, write it completely: if(!value1.isFloatValue() && !value2.isFloatValue() && !value1.isIntValue() && !value2.isIntValue())
There was a problem hiding this comment.
Well - I took it from the original code.
| continue; | ||
| } else { | ||
| if (value1.isTokValue() || value2.isTokValue()) | ||
| break; | ||
| } | ||
| bool error = false; | ||
| if (result.isFloatValue()) { | ||
| if (isFloat) { |
There was a problem hiding this comment.
Actually, looking at the test, we just need to run calculate as both float if one operand is float and convert to the result type int or float at the end.
Also rather than use lambdas(like intValue1) to do the conversion we should instead update the getIntValue and getFloatValue methods to do the conversion. This should make the code much simpler.
There was a problem hiding this comment.
This fixes the regression introduced by my change. Thanks.
I will take a look into the remaining false negatives tomorrow. There is also a few test variations I still like to add.
There was a problem hiding this comment.
It looks like everything has already been fixed. The existing false negatives do not appear to be related to the commit in question but a much earlier one. Will clean up the tests tomorrow.
firewave
force-pushed
the
fp-comp-x
branch
5 times, most recently
from
148ce7f to
e5a6758
Compare
|
The tests are clean now but there is some weird behavior that I get a warning on the CLI but not in the tests. |
firewave
force-pushed
the
fp-comp-x
branch
2 times, most recently
from
95b46f2 to
47cdc07
Compare
|
I think I finally have figured this out. Sorry for all the noise. This still leaves the https://trac.cppcheck.net/ticket/11200 regression but I guess that can be looked at in a follow-up. |
|
The case in Like the input we end up doing The problem is that with these changes |
| if (error) | ||
| *error = true; | ||
| return R{}; | ||
| } | ||
| return wrap(x / y); | ||
| case '%': | ||
| if (isZero(MathLib::bigint(y)) || (std::is_integral<T>{} && std::is_signed<T>{} && isEqual(y, T(-1)) && isEqual(x, std::numeric_limits<T>::min()))) { | ||
| if (isZero(MathLib::bigint(y)) || (std::is_signed<T>{} && MathLib::bigint(y) < 0)) { |
There was a problem hiding this comment.
@chrchr-github This crash fix from #5587 was way too specific.
firewave
changed the title
|
@pfultz2 please have another look. |
lib/vf_settokenvalue.cpp
Outdated
| continue; | ||
| } else { | ||
| if (value1.isTokValue() || value2.isTokValue()) | ||
| break; | ||
| } | ||
| bool error = false; | ||
| auto val = calculate(parent->str(), floatValue1, floatValue2, &error); |
There was a problem hiding this comment.
You can't calculate the integral values using floating-point. If both value1 and value2 are integrals then you should use the intvalue to calculate.
There was a problem hiding this comment.
But it works... 😃
Actually, looking at the test, we just need to run calculate as both float if one operand is float and convert to the result type int or float at the end.
Reading this again I see I got it wrong. So re-introduce the previous calculation code but also keep the assignment code I added. Will do later.
There was a problem hiding this comment.
Adjusted it and it still passed locally. Duplicated some code for now.
Co-authored-by: Paul Fultz II <pfultz2@yahoo.com>
|
@pfultz2 Anything left to do? I would like to get this merged so I can proceed looking into the remaining issues. |
No description provided.